Skip to content

Always treat RETURNING as supported by SQL engines #19047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Oct 24, 2025
Merged

Always treat RETURNING as supported by SQL engines #19047

merged 14 commits into from
Oct 24, 2025

Conversation

@AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Oct 13, 2025 •
edited by MadLittleMods
Loading

Can do this now that SQLite 3.35.0 added support for RETURNING.

The RETURNING syntax has been supported by SQLite since version 3.35.0 (2021-03-12).

-- https://sqlite.org/lang_returning.html

This also bumps the minimum supported SQLite version according to Synapse's deprecation policy.

Fix #17577

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

@AndrewFerr AndrewFerr requested a review from a team as a code owner October 13, 2025 02:34
@github-actions github-actions bot deployed to PR Documentation Preview October 13, 2025 02:35 Active
@github-actions github-actions bot deployed to PR Documentation Preview October 13, 2025 02:59 Active
@reivilibre
Copy link
Contributor

seems good, CI is failing though (maybe a missed usage of supports_returning?)

@github-actions github-actions bot deployed to PR Documentation Preview October 14, 2025 03:52 Active
@AndrewFerr
Use bookworm for sytest
ea3ec59 
as bookworm is what bumped libsqlite
@github-actions github-actions bot deployed to PR Documentation Preview October 15, 2025 21:58 Active
AndrewFerr
AndrewFerr commented Oct 20, 2025
View reviewed changes
@MadLittleMods MadLittleMods removed the request for review from a team October 22, 2025 17:42
@AndrewFerr
Merge branch 'develop' into af/sql-always-use-returning
c4a2def
@AndrewFerr
Remove comment about Sytest's Python version
9970475 
as it isn't all that relevant, and must be kept up to date.
Implicitly trust that the synapse-sytest image uses an appropriate
Python version.
@github-actions github-actions bot deployed to PR Documentation Preview October 23, 2025 20:12 Active
@github-actions github-actions bot deployed to PR Documentation Preview October 23, 2025 20:17 Active
@AndrewFerr
Copy link
Member Author

AndrewFerr commented Oct 23, 2025 •
edited
Loading

The SQLite sytest is working now, but trial-olddeps still needs to pull in libsqlite3-0 >=3.40.

For context, trial-olddeps runs on Ubuntu 22.04 (Jammy), which ships 3.37.2 (source). Meanwhile, 24.04 (Noble) ships 3.45.1 (source). Is 22.04 still not too old for Synapse to support?

EDIT: Ubuntu 22.02 ships Python 3.10, and is still in support, so it's a good idea for Synapse to still support it. Should Synapse's deprecation policy could be relaxed to allow the minimum libsqlite3-0 version between Debian oldstable and the oldest active Ubuntu LTS release? That would be sufficient for this PR, since SQLite's RETURNING support was added in 3.35.0.

@AndrewFerr
Bump libsqlite3-0 patch version
a198080
@AndrewFerr
Support libsqlite3-0 provided by Ubuntu 22.04 LTS
cba64e3 
This is older than what's provided by Debian bookworm (oldstable), but
this allows Synapse to keep supporting Ubuntu 22.04
@github-actions github-actions bot deployed to PR Documentation Preview October 24, 2025 15:10 Active
@AndrewFerr
Copy link
Member Author

Loosening the minimum SQLite version indeed fixes trial-olddeps.

The remaining sytest failure is on a flaky /purge_history test that has failed in other PRs, and is a false positive here.

MadLittleMods
MadLittleMods approved these changes Oct 24, 2025
View reviewed changes
docs/deprecation_policy.md
Comment on lines 23 to 25
The oldest supported version of SQLite is the version
[provided](https://packages.debian.org/bullseye/libsqlite3-0) by
[provided](https://packages.debian.org/oldstable/libsqlite3-0) by
[Debian oldstable](https://wiki.debian.org/DebianOldStable).
Copy link
Contributor

@MadLittleMods MadLittleMods Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubuntu 22.02 ships Python 3.10, and is still in support, so it's a good idea for Synapse to still support it. Should Synapse's deprecation policy could be relaxed to allow the minimum libsqlite3-0 version between Debian oldstable and the oldest active Ubuntu LTS release? That would be sufficient for this PR, since SQLite's RETURNING support was added in 3.35.0.

-- @AndrewFerr, #19047 (comment)

To break it down for myself:

  • Currently, the deprecation policy simply follows whatever version Debian oldstable ships for SQLite.
  • Debian oldstable ships with SQLite 3.40.1
  • Ubuntu 22.02 ships with SQLite 3.37.2 (older than Debian oldstable)
  • Ubuntu 22.02 is a supported LTS until April 2027
  • We even use Ubuntu 22.02 ourselves in our own trial-olddeps CI workflow

Your suggestion to update our deprecation policy to use the minimum between Debian oldstable and supported Ubuntu LTS releases sounds reasonable to me 👍

I'm concerned it locks us down too much in case we want to jump ("our primary focus is on the maintainability and progress of Synapse itself") but may be fine.

In any case as something non-committal, we can word it as "at-least Debian oldstable" and "best-effort support Ubuntu LTS" if we're not using new features which require it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can merge this PR now and we can tackle this in a follow-up PR where the context can be more focused ⏩

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case as something non-committal, we can word it as "at-least Debian oldstable" and "best-effort support Ubuntu LTS" if we're not using new features which require it.

I like this idea.

@github-actions github-actions bot deployed to PR Documentation Preview October 24, 2025 16:05 Active
@MadLittleMods MadLittleMods merged commit 9d81bb7 into element-hq:develop Oct 24, 2025
104 of 110 checks passed
@AndrewFerr AndrewFerr deleted the sql-always-use-returning branch October 31, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MadLittleMods MadLittleMods MadLittleMods approved these changes

Assignees

No one assigned

Labels

A-Database

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Always use RETURNING once the minimum supported SQLite version is 3.35.0

3 participants

@AndrewFerr @reivilibre @MadLittleMods